Skip to content

refactor: extract terminal emulator behind a backend trait - #78

Open
aymanbagabas wants to merge 15 commits into
microsoft:mainfrom
aymanbagabas:refactor/terminal-emulator-backend-trait
Open

refactor: extract terminal emulator behind a backend trait#78
aymanbagabas wants to merge 15 commits into
microsoft:mainfrom
aymanbagabas:refactor/terminal-emulator-backend-trait

Conversation

@aymanbagabas

@aymanbagabas aymanbagabas commented Jul 29, 2026

Copy link
Copy Markdown
Member

Puts the terminal emulator behind an Emulator trait so additional backends (libghostty, xterm.js) can be added without touching the daemon, render, or assert layers, then makes the neutral grid vocabulary lossless enough for those backends to agree on.

The seam

  • EmuAlacrittyEmu, now the first impl Emulator; neutral grid types moved to terminal::cell
  • CommandTracker lifted out of the emulator into TermState — OSC 133/633/7 parsing is backend-neutral, so every backend gets identical shell integration by construction rather than reimplementing it
  • adds emulator_conformance_tests!, backend-agnostic tests any new backend has to pass

The cell model

The vocabulary flattened away detail the emulator had already parsed, and each loss surfaced downstream:

  • Color::Default was indistinguishable from ANSI 0, so expect --fg 0 matched a default-colored cell and reported it as black when the theme paints it light gray. Colors are Option<Color>; a default cell matches nothing.
  • A blank cell and the second column of a double-width character were both "", and text extraction substituted a space for each — inventing a column, so 你a extracted as 你 a and shifted every column after it. Blanks hold a real space, "" means continuation, extraction skips it.
  • Five underline styles collapsed into one bool and the underline color was dropped. Option<Underline> keeps both.
  • Palette slots 0-15 are themeable and 16-255 are fixed, but both landed in Color::Idx. Now Color::Named and Color::Idx, split by numeric range rather than by how the escape sequence spelled it, so a backend that only carries a palette index agrees with one that carries a name.

Attributes move into a bitflags set, which adds blink and reduces the per-cell style comparison in the render and monitor loops to one integer compare. ch becomes a CompactString, inline up to 24 bytes, so the grid stops heap-allocating per cell and can hold combining marks the backend was discarding.

Wire format

Colors are unchanged: named and indexed both serialize to their palette index. cells was reporting four of seven attributes, so dim, invisible, strike and blink are added along with underline_style and underline_color. blink is always false from alacritty, which parses SGR 5/6/25 and discards them, but ghostty and xterm.js both track it. A continuation cell now reports "" rather than a space; the binding types document it.

aymanbagabas and others added 3 commits July 29, 2026 14:56
The grid was produced directly by an alacritty-specific `Emu` struct, so
supporting another emulator meant editing every consumer. Split the seam:

- `terminal/cell.rs` holds the neutral grid vocabulary (`EmuCell`, `Color`,
  `rows_to_strings`) with no emulator dependency
- `terminal/emu.rs` defines the `Emulator` trait, the whole contract between
  the PTY and the grid
- `terminal/alacritty.rs` (moved from `emu.rs`) implements it

Command/exit/cwd tracking moves out of the emulator into `TermState`. It is
derived from the raw PTY byte stream and never touched the alacritty grid, so
keeping it outside the trait means every backend reports identical shell
integration by construction instead of reimplementing it. `integration.rs` is
unchanged.

Consumers already spoke `EmuCell`/`Color` rather than alacritty types, so this
is import churn for them. No behavior change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Swapping the emulator behind the `Emulator` trait silently changes what
`expect`, `snapshot`, and the SVG renderer see, and the grid had no tests at
all. Add a macro-generated suite that every backend opts into with one line,
so a second backend is verified against the same contract as the first.

Covers grid shape, blank-vs-space cells, SGR attributes, palette/RGB/default
colors, wide-char spacers, cursor position and clamping, resize, scrollback
retention and ordering, alternate screen, erase, PTY write-back for DSR, and
escape sequences split across reads.

Each case is its own test, so a failure names the part of the contract that
broke. 17 cases pass against the alacritty backend.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Review found several cases asserted far less than their names claimed, so a
materially broken backend could pass the suite that exists to gate backends:

- cursor clamping accepted any in-bounds value, including (0, 0); now pins the
  exact bottom-right cell
- DSR accepted any CSI prefix; now pins the exact cursor position report
- erase and blank cells checked rendered text only, so a backend that dropped
  colors or attributes passed; now compares whole cells against the default
- resize checked only the reported size, never that content survived
- full_rows compared lengths, not contents, and never pinned that history is
  followed by exactly the viewport, which `grid(full = true)` relies on

Also covers areas that were untested: split UTF-8 across reads (the reader
uses a fixed 8 KiB buffer, so real output splits codepoints), autowrap, tabs,
bare CR overwrite, backspace, erase-to-end-of-line, and the scrollback limit,
which no case exercised despite the daemon requesting 5000 rows.

Verified by mutation: a backend that reports the cursor at the origin, renders
blank cells as spaces, or drops scrollback now fails 8 cases; before it passed
all of them.

Tabs pin only cursor advance and landing column. Alacritty stores a literal
tab in the skipped cells and other emulators store blanks, so asserting the
rendered text there would fail a correct backend.

The macro no longer emits `use` statements into the caller's module; a second
backend whose test module imports Color or Emulator itself would have hit
E0252.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas
aymanbagabas marked this pull request as draft July 29, 2026 20:30
aymanbagabas and others added 3 commits July 29, 2026 16:53
The grid vocabulary flattened away detail that the emulator had already
parsed, and each loss surfaced somewhere downstream:

- `Color::Default` was indistinguishable from ANSI 0, so `expect --fg 0`
  matched a default-colored cell and claimed it was black when the theme
  paints it light gray. Colors are now `Option<Color>`, and a default cell
  matches no expected value at all.

- Palette slots 0-15 are themeable and 16-255 are fixed, but both landed
  in `Color::Idx`, leaving every consumer to re-derive the split. They are
  now `Color::Named` and `Color::Idx`, divided by numeric range rather than
  by how the escape sequence spelled it, so backends that carry only a
  palette index agree with backends that carry a name.

- Five distinct underline styles collapsed into one bool, and the
  underline's own color was dropped entirely. `Option<Underline>` keeps
  both.

- A blank cell and the second column of a double-width character were both
  the empty string, and text extraction substituted a space for each. That
  invented a column: `你a` extracted as `你 a`, shifting every subsequent
  column and breaking text matching. Blanks now hold a real space and the
  empty string means continuation, which extraction skips.

Attributes move into a `bitflags` set, which adds blink and turns the
per-cell style comparison in the render and monitor loops into a single
integer compare. `ch` becomes a `CompactString`, inline for anything up to
24 bytes, so the grid no longer heap-allocates per cell and can hold the
combining marks the backend was discarding.

The JSON wire format is unchanged: named and indexed colors both serialize
to their palette index and underline stays a boolean, so the JS and Python
bindings keep working. The one visible difference is that a continuation
cell now reports `""` instead of a space, which the binding types document.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The `cells` response emitted four of the seven boolean attributes the
neutral model carries. `dim`, `invisible` and `strike` were parsed, stored
and then dropped at the wire, and `blink` had nowhere to go at all.

A client should be written against the vocabulary, not against whichever
backend happens to be compiled in, so all seven are reported now. `blink`
is always false from alacritty, which parses SGR 5/6/25 and discards them,
but ghostty (`Style.Flags.blink`) and xterm.js (`FgFlags.BLINK`) both track
it and will fill it in without any client change.

Underline style and color reach the wire for the same reason, as
`underline_style` and `underline_color` alongside the existing boolean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The rationale on `NamedColor` claimed xterm.js carries only palette
indices. It does not: `CM_P16` and `CM_P256` are distinct color modes, so
xterm.js preserves `SGR 31` vs `SGR 38;5;1` exactly as alacritty does.

The reason to drop that distinction is ghostty, whose `Style.Color` has a
single `.palette` variant and cannot reproduce it, plus the fact that
nothing here consumes it: bold-to-bright keys off the index, not off how
the color was spelled.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas
aymanbagabas marked this pull request as ready for review July 29, 2026 21:09

@cpendery cpendery left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small comments on specific code, also generally if expect --fg 0 isn't working for the default color for the cell, we should enable the user to do expect --fg default since default is supported for snapshots & error messages

Comment thread src/monitor.rs Outdated
Comment thread src/terminal/alacritty.rs Outdated
Comment thread src/terminal/alacritty.rs Outdated
Comment thread bindings/js/src/types.ts Outdated
Comment thread src/assert/color.rs Outdated
aymanbagabas and others added 4 commits July 31, 2026 08:41
A wide char that does not fit in the last column wraps whole to the next
row. Alacritty flags the column it vacated with LEADING_WIDE_CHAR_SPACER,
which the backend was folding into the same continuation marker it uses
for WIDE_CHAR_SPACER, the genuine second half of a wide char.

The two are opposites. A continuation renders as nothing because its
glyph already covered the column; the filler owns its column and has to
render as a blank. Conflating them dropped a column, so a 5-wide grid
showing "abcd你" reported row 0 as "abcd", and every consumer that counts
columns, snapshots included, saw the row short by one.

Add wrap coverage to the conformance suite so the next backend has to
draw the same distinction.

While in the same expression, build the grapheme with to_compact_string
rather than CompactString::from(char::to_string), which allocated a
String only to copy out of it. Removing that allocation was the point of
moving to CompactString in the first place.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The monitor wrote ";58;2::{r}:{g}:{b}", which is neither valid form: the
semicolon ends the parameter, so a terminal reads 58 as a bare "set
underline color" and takes whatever follows as its arguments. The
foreground color emitted after it was swallowed whole, leaving the text
uncolored and the underline painted the color the text should have been.

SGR 58 takes colon-joined subparameters. Emit "58:2::{r}:{g}:{b}" and
"58:5:{n}", matching the ";4:{sub}" already emitted for the style.

Nothing caught this because sgr output only ever went to a real terminal,
where a malformed escape reassigns parameters silently instead of
failing. Feed it back through an emulator and compare the style that
comes out.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
underline_style was null when a cell had no underline and underline_color
was null when the underline followed the text color, so a client had to
null-check two fields whose neighbours, fg and bg, already spell the same
idea as the string "default".

Give both a value instead. An absent underline is the "none" style, and
an underline with no color of its own is "default", matching fg and bg.
A client can now switch on the style string and compare the color the
same way it compares any other, and the underline bool stays as a
shorthand for style != "none".

Move the style's wire name onto UnderlineStyle so the mapping lives with
the type the next backend will reuse rather than in the daemon.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
A cell that sets no color of its own matched no --fg or --bg value, on
purpose: which color it paints is the viewer's theme's choice, so --fg 0
claiming it is black would be a lie. That left no way to assert on it at
all, even though snapshots and failure messages had been calling it
"default" all along.

Accept that same word as an expected value. --fg default passes only for
a cell that set no foreground, and a cell that did set one now fails
against it, so the assertion reads both ways.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas

Copy link
Copy Markdown
Member Author

All six addressed, pushed as four commits.

expect --fg default — you were right that this was the missing half. Snapshots and failure messages had been calling it default all along, but there was no way to assert on it, so a default cell matched nothing. Expected::Default now accepts the same word (eff190f): --fg default passes only when the cell set no foreground, and a cell that did set one fails against it, so it reads both ways. Refusing --fg 0 for a default cell stays refused — which color the theme paints it is not something the grid knows.

Two of your comments were real bugs, both confirmed before fixing:

  • The SGR one was worse than malformed. Round-tripping the old output through the emulator gave fg=None, underline.color=Rgb(38, 5, 1) — the ; ended the parameter and the underline color ate the 38;5;1 foreground. It went unnoticed because sgr only ever wrote to a real terminal, where a bad escape silently reassigns parameters rather than failing.
  • The spacer one dropped a column: abcd你 on a 5-wide grid reported row 0 as "abcd".

Both now have the check that was missing — an SGR round-trip test, and wrap tests in the shared conformance macro so ghostty and xterm.js have to get this right too.

LEADING_WIDE_CHAR_SPACER + wrap tests + to_compact_string 322519f
SGR 58 colon form + round-trip test 6cbeb08
Non-nullable underline_style / underline_color f521c18
expect --fg default + comment cleanup eff190f

85 tests pass, clippy and fmt clean, both bindings build.

aymanbagabas and others added 3 commits July 31, 2026 12:10
Snapshot serialization rendered a continuation cell as a space. A wide
character already spans both of its columns, so the filler pushed every
later column one to the right and left the content line one column wider
than the frame drawn around it:

    ╭──────╮
    │a你 b  │
    ╰──────╯

Every snapshot holding a wide character was written that way, and
--update baked the misaligned form in as the baseline to compare against.

Contribute the grapheme verbatim, the same rule rows_to_strings follows.
The two had diverged because this loop is hand-rolled: it also builds the
per-cell style shift map, so it could not call the shared helper and
grew its own answer to what a continuation renders as.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The cell held Option<Underline { style, color }>, so "not underlined"
had two spellings and every reader went through a map or and_then to
reach either half. The daemon, the monitor, the SVG renderer and the
snapshot serializer all paid that toll, and the wire format already
disagreed with the model: it reports a "none" style and a "default"
color rather than nulls.

Store the shape and the color side by side instead. UnderlineStyle grows
a None variant and becomes its own Default, so the cell reads
`underline` and `underline_color` the same way it reads `fg` and `bg`,
and the wire form is now a direct projection of the model rather than a
translation of it.

This also fixes a quiet loss. The underline color was reachable only
through the underline, so a cell that set SGR 58 without SGR 4, or that
cleared the shape with SGR 24, reported no color at all. A terminal
tracks the two separately and only a full reset clears both; the flat
model can say so, and a conformance test now pins it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
The wrap fix and the snapshot fix were each tested on their own: the grid
in the conformance suite, `serialize` on hand-built rows. Neither ran the
serializer over a grid a backend actually produced, which is precisely
the pairing that broke. Both halves were individually right and the frame
still came out a column wide, because they disagreed about what a
continuation renders as.

Run the real serializer against a real wrapped grid so a backend has to
get the composition right, not just its own half. Reintroducing either
bug fails this test.

Rename conformance_wrap_snapshot to conformance_wrap_grid_over_several_rows;
it dumps the grid and never touched the snapshot code its name claimed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas
aymanbagabas requested a review from cpendery July 31, 2026 17:01
Comment thread src/assert/color.rs Outdated
Comment thread src/assert/snapshot.rs Outdated
aymanbagabas and others added 2 commits August 3, 2026 09:54
The literal was spelled out at four production sites across three
modules, all meaning the same wire value.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Snapshots stored a bare "is underlined", so a curly underline turning
single kept passing. The style name is recorded instead, matching the
vocabulary the daemon already reports in `underline_style`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
@aymanbagabas
aymanbagabas requested a review from cpendery August 3, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants